You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
const x at the end of a file reports position.length 0. break \u{79} covers \u. export { \u{69}f } reports Expected identifier but found "\u".
No user reported this. A comparison with esbuild found it.
Fix
Delete the copy. 44 call sites in the parser, resolver and linker call Source::range_of_identifier with the same arguments.
The import.meta site stores the length of import. No scan logic is new: src/ast/lib.rs equals main.
Verified: test/bundler/transpiler/transpiler.test.js, test/bundler/bun-build-api.test.ts (48 new rows, main fails 42). Also bundler_edgecase.test.ts, esbuild/importstar.test.ts.
The offset of a diagnostic never changes. Only the length does.
Three readers use the length:
error.position.length of a BuildMessage (Bun.Transpiler, Bun.build logs, a module that fails to load)
the error overlay of the dev server (src/runtime/bake/client/overlay.ts:725)
the AOT span (src/aot/lower/transpiled.rs:105)
The terminal prints one ^ at the start of the range and does not read the length. Terminal output changes only in three message texts:
export { \u{69}f };: Expected identifier but found "\u" is now Expected identifier but found "\u{69}f".
export { x, typeof at the end of a file: Expected identifier but found "typeo" is now "typeof".
export default interface at the end of a .js file: Unexpected "interfac" is now Unexpected "interface".
No existing test asserted the old texts.
esbuild
RangeOfIdentifier in esbuild (internal/js_lexer/js_lexer.go:631) gives the same ranges as the surviving body for the two escape steps. For an identifier that ends the file it returns length 0, because it falls through to RangeOfString. Bun covers the identifier. The unit test identifier_at_eof_includes_last_codepoint from #31783 pins that.
How to replay the edit
In the 10 files that git grep -l 'lexer::range_of_identifier\|lex::range_of_identifier' -- src lists, replace X::range_of_identifier(SRC, LOC) with SRC.range_of_identifier(LOC). Two calls span lines (p.rs:3659, parse/parse_property.rs:153). One call in lexer.rs:1875 has no path prefix.
Write the literal at parse/parse_import_export.rs:21 by hand. parse_import_expr has two callers. Both pass the loc of a T::TImport token. An escaped keyword lexes as T::TEscapedKeyword, so that token is always the bytes import.
Delete lexer.rs:3351-3409. Remove the five imports that the compiler then names (LinkerContext.rs, parse_import_export.rs, scan_symbols.rs, visit/mod.rs, package_json.rs). Run cargo fmt --all.
A script compared the calls before and after. 45 free-function calls became 44 method calls with the same source and loc arguments, token for token. The 45th is the literal.
Tests
42 parser rows and 6 linker rows are new. A release build of main 7ca0042 fails 36 and 6. The 6 parser rows that pass are the group "in a spelling that was already right".
The linker rows are in bun-build-api.test.ts because only the JS API exposes a length.
Each fixed step is pinned. I removed one step at a time from src/ast/lib.rs. That run used an earlier set of 39 parser rows and 4 linker rows. Without the leading-escape skip, 20 and 2 fail. Without cursor.width = 0, 18 and 2. Without the end-of-file width, 7 and 0.
bun bd test test/bundler/transpiler/transpiler.test.js: 300 pass, 0 fail.
bun bd test test/bundler/bun-build-api.test.ts: 96 pass and 1 timeout. The timeout is the existing stress test Bun.build can be called thousands of times in one process without crashing. It needs 400 builds in 180 s, and the ASAN debug build here takes about 1 s for each. The release builds of main and of this branch both run 40 of those builds in 1.3 s.
Also run on the debug build, all pass: bundler_edgecase.test.ts, esbuild/importstar.test.ts, parse-error-column.test.ts, resolve-error.test.ts, assign-to-import.test.ts, decorators.test.ts, scope-mismatch-panic.test.ts, and the regression tests 23649 and 012039.
Measurements (release builds of main 7ca0042 and of this branch, --profile=release)
Instructions per scan (gdb single-step from entry to return), with one conditional branch fewer in each:
function eval(){}: 131 to 138
var arguments = 1: 226 to 238
require(x): 188 to 198
import(x): 169 to 178
import.meta: 1 scan of 169 instructions to 0 scans. A file of 1,000 import.meta statements hits the scan 1,000 times on main and 0 times now.
.text: 110,627,497 to 110,625,705 bytes (llvm-size). bun_js_parser::lexer::range_of_identifier (1,737 bytes) is gone. <bun_ast::Source>::range_of_identifier is 2,042 bytes in both.
Calls out of the surviving body (objdump): no allocator call. The targets are panic_bounds_check, unwrap_failed, and memcpy on the non-ASCII path.
A name that starts with a 1,000-byte escape: 104 to 12,086 instructions per diagnostic. The copy stopped after 2 bytes. With the escape after the first character: 6,081 to 8,091.
Worst case tried: 20,000 diagnostics whose note points at one name that starts with a 400,000-byte escape. The median goes from 5.07 s to 8.15 s (10 interleaved runs each, spread 0.7 s).
A matrix of 819 spellings and 5 terminators: wrong ranges at a label diagnostic 1,771 to 0 of 4,095. At a redeclaration, which already used the surviving body, 0 to 0.
Still wrong after this PR (same output on main and on this branch, lines as on main)
Five private-name ranges take the length from the decoded name, as esbuild does (visit/visit_expr.rs:994, :1011, :1027, :1043, visit/visit_binary.rs:743). this.#\u{62} covers #\. In the same matrix 2,805 of 4,095 rows are wrong.
parse/parse_prefix.rs:400 also starts at the object. delete this.#bcdef covers this.#.
A name that is a string literal gets length 0. Examples are export { a as "x", b as "x" } and a second "__proto__" key. export { "a b" } reports Expected identifier but found "". esbuild falls back to RangeOfString.
continue to a label that is not a loop underlines continue, not the label (visit/visit_stmt.rs:1403).
A setter with no parameter underlines nothing (parse/parse_property.rs:153).
TypeScript import { type as eval } reports Cannot use eval as an identifier here with length 0 (parse/parse_import_export.rs:187).
import { x as eval } underlines x, not eval (parse/parse_import_export.rs:230).
no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts
…dentifier
The scan that finds the end of an identifier for a diagnostic existed
twice. #31783 fixed three steps in `bun_ast::range_of_identifier`: a
name that starts with a `\u{...}` escape, the byte after the closing
brace of an escape, and a name that ends the file. The copy in
lexer.rs did not get those fixes, and 45 call sites still called it.
Delete the copy. 44 call sites in the parser, the resolver and the
linker call `Source::range_of_identifier` with the same arguments. The
`import.meta` site stores the length of the `import` keyword and does
not scan.
Reproduced on main 7ca0042 with a release build and Bun.Transpiler#transformSync. const x with no final newline reports The constant "x" must be initialized with position.length 0. for (;;) { break \u{79}; } reports There is no containing label named "y" with position.length 2, which covers \u. export { \u{69}f }; reports Expected identifier but found "\u".
With this branch, the same three inputs report position.length 1, position.length 6 and Expected identifier but found "\u{69}f".
No actionable comments were generated in the recent review.
Walkthrough
Parser, linker, and resolver diagnostic locations now use Source::range_of_identifier instead of the lexer helper. The helper was removed. New tests check diagnostic spans for escaped and unescaped identifiers.
Changes
Identifier diagnostic ranges
Layer / File(s)
Summary
Parser diagnostic range migration src/js_parser/lexer.rs, src/js_parser/p.rs, src/js_parser/parse/*, src/js_parser/scan/*, src/js_parser/visit/*
Parser diagnostic call sites now obtain identifier ranges through Source::range_of_identifier. The free lexer helper was removed.
Linker and resolver diagnostic ranges src/bundler/LinkerContext.rs, src/resolver/package_json.rs
Linker and package.json diagnostics now obtain identifier ranges through Source::range_of_identifier.
Reviewing files that changed from the base of the PR and between 7ca0042 and 45b5078.
Files selected for processing: 13
src/bundler/LinkerContext.rs
src/js_parser/lexer.rs
src/js_parser/p.rs
src/js_parser/parse/parse_import_export.rs
src/js_parser/parse/parse_property.rs
src/js_parser/parse/parse_stmt.rs
src/js_parser/scan/scan_symbols.rs
src/js_parser/visit/mod.rs
src/js_parser/visit/visit_expr.rs
src/js_parser/visit/visit_stmt.rs
src/resolver/package_json.rs
test/bundler/bun-build-api.test.ts
test/bundler/transpiler/transpiler.test.js
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review.
The reason will be displayed to describe this comment to others. Learn more.
LGTM — a mechanical dedup that routes every identifier-range diagnostic through the already-fixed Source::range_of_identifier, with tests that pin each corrected case.
What was reviewed:
All 44 swapped call sites pass the same source/loc as before; no caller of the deleted lexer copy remains, and the leftover js_lexer/CodepointIterator imports are still live.
The hard-coded len: 6 for import.meta: both callers of parse_import_expr pass the loc of a T::TImport token, and an escaped import lexes as T::TEscapedKeyword, so the token is always the 6 bytes import.
The surviving body in src/ast/lib.rs is unchanged here, already had four callers and unit tests; the leading-\u{ scan is bounded by source length and only runs on diagnostic paths.
New tests assert exact message plus covered-text pairs via toEqual, and no existing test asserted the three old truncated message strings.
Optional nit: the doc comment above range_of_identifier in src/ast/lib.rs:1089 ("Was bun_js_parser::lexer::rangeOfIdentifier. Moved into logger to break logger→js_parser") now describes a copy that no longer exists; worth trimming while you're here.
Extended reasoning...
The PR deletes the duplicate range_of_identifier in src/js_parser/lexer.rs and rewrites 44 call sites across the parser, linker and resolver to call the pre-existing Source::range_of_identifier method, plus one site that replaces a scan of the import keyword with a fixed 6-byte range. It touches no security-sensitive surface; the only effect is the length of diagnostic ranges (error.position.length) and three message texts that were previously truncated. Approval was decided by the fact that every swap is argument-identical, the single semantic change was verified against both callers and the lexer's escaped-keyword handling, the surviving helper is untouched and already tested, and the added test.each suites assert exact values and cover leading/trailing escapes, EOF and non-ASCII. No CODEOWNERS entry applies and the timeline shows no outstanding reviewer objections.
Thanks for the review. I left the doc comment at src/ast/lib.rs:1089 as it is on main:
The note names the Zig function rangeOfIdentifier, not the Rust copy that this PR deletes. It records why the scan lives in bun_ast and not in the lexer: bun_ast cannot depend on bun_js_parser. That is still true.
This PR keeps src/ast/lib.rs identical to main, so the surviving scan and its unit tests are untouched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
Problem
src/ast/lib.rs:1093, not the copy atsrc/js_parser/lexer.rs:3351, which 45 of 49 call sites use.const xat the end of a file reportsposition.length0.break \u{79}covers\u.export { \u{69}f }reportsExpected identifier but found "\u".Fix
Source::range_of_identifierwith the same arguments.import.metasite stores the length ofimport. No scan logic is new:src/ast/lib.rsequals main.test/bundler/transpiler/transpiler.test.js,test/bundler/bun-build-api.test.ts(48 new rows, main fails 42). Alsobundler_edgecase.test.ts,esbuild/importstar.test.ts.letas a name that a let, const or using declaration binds in a pattern or with an escape #48043, Refuse a macro import that is used outside of a call #46239, bundler: warn about direct eval in ES modules, with one guard for weird-code warnings #46890.Background
error.position.length.lexer.rsand a port of the fixes. The forwarder keeps a second public name. The port keeps two bodies.Downsides
eval: 131 to 138, gdb, release builds).\u{...}is scanned to its closing brace. A 1,000-byte escape costs 12,086 instructions, up from 104.Notes
The three steps the copy had wrong
const x(end of file)xfor (;;) break abc(end of file)ababcfor (;;) { break \u{79}; }\u\u{79}for (;;) { break a\u{62}; }a\u{62};a\u{62}export { a\u{62} as x };a\u{62} asa\u{62}for (;;) { break a\u{62}é; }éa\u{62}é;after the name, main is right.\u{...}there. The loop then stopped at{.loc.start < -1. The surviving body returnsRange::NONE.src/js_lexer.zig:3104) has the same three steps. When Resolve the audited TODO(port)/TODO(b2)/PORT NOTE backlog #31783 fixed the other copy, 41 call sites called this one. Now 45 do.What a user sees
error.position.lengthof aBuildMessage(Bun.Transpiler,Bun.buildlogs, a module that fails to load)src/runtime/bake/client/overlay.ts:725)src/aot/lower/transpiled.rs:105)^at the start of the range and does not read the length. Terminal output changes only in three message texts:export { \u{69}f };:Expected identifier but found "\u"is nowExpected identifier but found "\u{69}f".export { x, typeofat the end of a file:Expected identifier but found "typeo"is now"typeof".export default interfaceat the end of a.jsfile:Unexpected "interfac"is nowUnexpected "interface".esbuild
RangeOfIdentifierin esbuild (internal/js_lexer/js_lexer.go:631) gives the same ranges as the surviving body for the two escape steps. For an identifier that ends the file it returns length 0, because it falls through toRangeOfString. Bun covers the identifier. The unit testidentifier_at_eof_includes_last_codepointfrom #31783 pins that.How to replay the edit
git grep -l 'lexer::range_of_identifier\|lex::range_of_identifier' -- srclists, replaceX::range_of_identifier(SRC, LOC)withSRC.range_of_identifier(LOC). Two calls span lines (p.rs:3659,parse/parse_property.rs:153). One call inlexer.rs:1875has no path prefix.parse/parse_import_export.rs:21by hand.parse_import_exprhas two callers. Both pass the loc of aT::TImporttoken. An escaped keyword lexes asT::TEscapedKeyword, so that token is always the bytesimport.lexer.rs:3351-3409. Remove the five imports that the compiler then names (LinkerContext.rs,parse_import_export.rs,scan_symbols.rs,visit/mod.rs,package_json.rs). Runcargo fmt --all.Tests
bun-build-api.test.tsbecause only the JS API exposes a length.src/ast/lib.rs. That run used an earlier set of 39 parser rows and 4 linker rows. Without the leading-escape skip, 20 and 2 fail. Withoutcursor.width = 0, 18 and 2. Without the end-of-file width, 7 and 0.bun bd test test/bundler/transpiler/transpiler.test.js: 300 pass, 0 fail.bun bd test test/bundler/bun-build-api.test.ts: 96 pass and 1 timeout. The timeout is the existing stress testBun.build can be called thousands of times in one process without crashing. It needs 400 builds in 180 s, and the ASAN debug build here takes about 1 s for each. The release builds of main and of this branch both run 40 of those builds in 1.3 s.bundler_edgecase.test.ts,esbuild/importstar.test.ts,parse-error-column.test.ts,resolve-error.test.ts,assign-to-import.test.ts,decorators.test.ts,scope-mismatch-panic.test.ts, and the regression tests 23649 and 012039.Measurements (release builds of main 7ca0042 and of this branch,
--profile=release)function eval(){}: 131 to 138var arguments = 1: 226 to 238require(x): 188 to 198import(x): 169 to 178import.meta: 1 scan of 169 instructions to 0 scans. A file of 1,000import.metastatements hits the scan 1,000 times on main and 0 times now..text: 110,627,497 to 110,625,705 bytes (llvm-size).bun_js_parser::lexer::range_of_identifier(1,737 bytes) is gone.<bun_ast::Source>::range_of_identifieris 2,042 bytes in both.objdump): no allocator call. The targets arepanic_bounds_check,unwrap_failed, andmemcpyon the non-ASCII path.Still wrong after this PR (same output on main and on this branch, lines as on main)
visit/visit_expr.rs:994,:1011,:1027,:1043,visit/visit_binary.rs:743).this.#\u{62}covers#\. In the same matrix 2,805 of 4,095 rows are wrong.parse/parse_prefix.rs:400also starts at the object.delete this.#bcdefcoversthis.#.export { a as "x", b as "x" }and a second"__proto__"key.export { "a b" }reportsExpected identifier but found "". esbuild falls back toRangeOfString.continueto a label that is not a loop underlinescontinue, not the label (visit/visit_stmt.rs:1403).parse/parse_property.rs:153).import { type as eval }reportsCannot use eval as an identifier herewith length 0 (parse/parse_import_export.rs:187).import { x as eval }underlinesx, noteval(parse/parse_import_export.rs:230).awaiterror underlines the token afterawait(lexer.rs:1884, Irrelevant error: using await in non-async function #13097).hoist_symbolswrites its own redeclaration error, with no quotes and with the error on the first declaration (p.rs:3643).export default 1; export default function foo() {}underlinesfoo(parse/parse_stmt.rs:1158).import abc = require('x')stores no loc. A note aboutabcsits at offset 0 (parse/parse_stmt.rs:1635).Open PRs that call the deleted name
letas a name that a let, const or using declaration binds in a pattern or with an escape #48043, Refuse a macro import that is used outside of a call #46239 and bundler: warn about direct eval in ES modules, with one guard for weird-code warnings #46890 merge onto this branch with no text conflict and then do not compile. Each added call needsself.source.range_of_identifier(loc). The method is public on main today, so they can change before this lands.src/resolver/package_json.rs.usingdeclarations may appear (switch clauses, for heads,awaitnewline) #40813, js_parser: report missing await, yield and binding pattern early errors #40821, parser: report strict-mode early errors and accept legacy sloppy syntax #40827, js_parser: keep "use strict" and the directive prologue in the output #40838, js_parser: make the ESM/CJS classification and the module/exports bindings agree #40840, js_parser: report delete of a bare identifier in strict mode #40861, js_parser: reject a modifier or an escape in front of a class static block #44909, bundler: keep the CommonJS wrapper of a package that a file writes to through require() #46216, Implement dynamicimport.defer()(TC39 Stage 3) #31244, node: ESM resolver error-code parity — ERR_PACKAGE_PATH_NOT_EXPORTED and friends (+4 tests) #35711 and bundler: remove unsafe from bundle_v2, ParseTask, LinkerContext and the bundler thread pool #40438 also add or change a call. They already conflict with main.no test proof · iteration 0 · platform-specific test(s) that do not run on this machine, deferring to CI, which covers all platforms: test/bundler/bun-build-api.test.ts